Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Expression Evaluator and Script Watches to the debugger #1

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

rxlecky
Copy link

@rxlecky rxlecky commented Nov 7, 2019

Evaluator allows expressions to be evaluated in the current stack frame.
Watches keep track of a list of expressions.

The original PR: godotengine#26219
This is still a WIP. Testing and feedback is much appreciated.
CC @jahd2602

@rxlecky rxlecky requested a review from vnen as a code owner November 7, 2019 14:23
@jahd2602
Copy link
Member

jahd2602 commented Nov 7, 2019

Thanks @rxlecky!

I've been testing the commit and I found the following issues:

  1. The expression evaluator does not seem to recognize autoloads
  2. The expression evaluator does not let me select the text, especially when I wanted to start watching a variable I typed before (I know pressing the up key works too)
  3. Usability-wise, it is hard to see the watched variables ("Watches" tab) and step-over, because the step-over is all the way in the other tab "Debugger"
  4. There is this crash that happens only sometimes:
ERROR: execute: There was previously a parse error: Expected expression.
   At: core/math/expression.cpp:2125.
handle_crash: Program crashed with signal 11
Dumping the backtrace. Please include this when reporting the bug on https://github.com/godotengine/godot/issues
[1] /lib/x86_64-linux-gnu/libc.so.6(+0x43f60) [0x7faf10a45f60] (??:0)
[2] /home/jairo/GodotProjects/project/godot/bin/godot.x11.opt.tools.64() [0x21497b9] (/home/jairo/GodotProjects/project/godot/core/script_language.cpp:503)
[3] /home/jairo/GodotProjects/project/godot/bin/godot.x11.opt.tools.64() [0x7787af] (/home/jairo/GodotProjects/project/godot/modules/gdscript/gdscript_function.cpp:1574)
[4] /home/jairo/GodotProjects/project/godot/bin/godot.x11.opt.tools.64() [0x72baf0] (/home/jairo/GodotProjects/project/godot/modules/gdscript/gdscript.cpp:655)
[5] /home/jairo/GodotProjects/project/godot/bin/godot.x11.opt.tools.64() [0x2186e37] (/home/jairo/GodotProjects/project/godot/core/variant_call.cpp:1055 (discriminator 1))
[6] /home/jairo/GodotProjects/project/godot/bin/godot.x11.opt.tools.64() [0x789038] (/home/jairo/GodotProjects/project/godot/modules/gdscript/gdscript_function.cpp:1114)
[7] /home/jairo/GodotProjects/project/godot/bin/godot.x11.opt.tools.64() [0x7277b8] (/home/jairo/GodotProjects/project/godot/modules/gdscript/gdscript.cpp:1175)
[8] /home/jairo/GodotProjects/project/godot/bin/godot.x11.opt.tools.64() [0x20ed62f] (/home/jairo/GodotProjects/project/godot/core/object.cpp:921)
[9] /home/jairo/GodotProjects/project/godot/bin/godot.x11.opt.tools.64() [0x2186e37] (/home/jairo/GodotProjects/project/godot/core/variant_call.cpp:1055 (discriminator 1))
[10] /home/jairo/GodotProjects/project/godot/bin/godot.x11.opt.tools.64() [0x78a5e4] (/home/jairo/GodotProjects/project/godot/modules/gdscript/gdscript_function.cpp:1109)
[11] /home/jairo/GodotProjects/project/godot/bin/godot.x11.opt.tools.64() [0x7277b8] (/home/jairo/GodotProjects/project/godot/modules/gdscript/gdscript.cpp:1175)
[12] /home/jairo/GodotProjects/project/godot/bin/godot.x11.opt.tools.64() [0x20ed62f] (/home/jairo/GodotProjects/project/godot/core/object.cpp:921)
[13] /home/jairo/GodotProjects/project/godot/bin/godot.x11.opt.tools.64() [0x2186e37] (/home/jairo/GodotProjects/project/godot/core/variant_call.cpp:1055 (discriminator 1))
[14] /home/jairo/GodotProjects/project/godot/bin/godot.x11.opt.tools.64() [0x78a5e4] (/home/jairo/GodotProjects/project/godot/modules/gdscript/gdscript_function.cpp:1109)
[15] /home/jairo/GodotProjects/project/godot/bin/godot.x11.opt.tools.64() [0x7290e5] (/home/jairo/GodotProjects/project/godot/./core/variant.h:421)
[16] /home/jairo/GodotProjects/project/godot/bin/godot.x11.opt.tools.64() [0x1319df5] (/home/jairo/GodotProjects/project/godot/scene/main/node.cpp:136)
[17] /home/jairo/GodotProjects/project/godot/bin/godot.x11.opt.tools.64() [0xb6a988] (/home/jairo/GodotProjects/project/godot/./scene/2d/canvas_item.h:166)
[18] /home/jairo/GodotProjects/project/godot/bin/godot.x11.opt.tools.64() [0x20e8294] (/home/jairo/GodotProjects/project/godot/core/object.cpp:954)
[19] /home/jairo/GodotProjects/project/godot/bin/godot.x11.opt.tools.64() [0x1307447] (/home/jairo/GodotProjects/project/godot/scene/main/node.cpp:184)
[20] /home/jairo/GodotProjects/project/godot/bin/godot.x11.opt.tools.64() [0x13073a3] (/home/jairo/GodotProjects/project/godot/./core/cowdata.h:70)
[21] /home/jairo/GodotProjects/project/godot/bin/godot.x11.opt.tools.64() [0x1317fb7] (/home/jairo/GodotProjects/project/godot/scene/main/node.cpp:2548)
[22] /home/jairo/GodotProjects/project/godot/bin/godot.x11.opt.tools.64() [0x1348356] (/home/jairo/GodotProjects/project/godot/scene/main/scene_tree.cpp:457)
[23] /home/jairo/GodotProjects/project/godot/bin/godot.x11.opt.tools.64() [0x6b2850] (/home/jairo/GodotProjects/project/godot/platform/x11/os_x11.cpp:2993)
[24] /home/jairo/GodotProjects/project/godot/bin/godot.x11.opt.tools.64(main+0xd8) [0x6a5478] (/home/jairo/GodotProjects/project/godot/platform/x11/godot_x11.cpp:55)
[25] /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xeb) [0x7faf10a28b6b] (??:0)
[26] /home/jairo/GodotProjects/project/godot/bin/godot.x11.opt.tools.64() [0x6a68aa] (??:?)
-- END OF BACKTRACE --

If there is anything I can do to help please let me know.

@rxlecky
Copy link
Author

rxlecky commented Nov 9, 2019

Thanks @jahd2602 for reporting the problems you are having, this really helps! I opened an issue for each of them respectively in my repo. When you encounter problems just open new issues. It would be also useful to mention how severely given issue impacts you to help me prioritize my work.

Also, I just want to ensure I understand the second issue from your list correctly. Is the problem that you can not select and copy from the log in the evaluator? If that's the case, as you mentioned, the workaround would currently be to use arrow up to find the correct expression and then press the Watch button. I agree that you should be able to copy from the log, though.

@jahd2602
Copy link
Member

@rxlecky I will open new issues as soon as I find them.

Is the problem that you can not select and copy from the log in the evaluator?

Exactly, I am aware of the workaround but still, I think new users will not get it immediately and might get frustrated.

It would be also useful to mention how severely given issue impacts you to help me prioritize my work.

Ok from the 4 items I mentioned, the order of importance would be 4 (most important), 1, 3 and 2 (as you mentioned, there is currently a workaround so we can leave it last).

@jahd2602
Copy link
Member

@rxlecky I just tested https://github.com/JavaryGames/godot/pull/1/files/3ac33d0ea0ca8fd8acf6bc9f707f41b7a0d368c8..b85c56ff791dd28f93262cd2769c5dce38fddf70 and indeed the crash does not seem to happen anymore!

Also, I was very impressed that I could watch expressions like array.size() and it updated live, it is just amazing!

@jahd2602
Copy link
Member

jahd2602 commented Nov 11, 2019

@rxlecky I also noticed something: When canceling the execution of the game, the watched expressions are still there, which is great. However, when playing the game again, they stop working. I guess because the instance ids are gone. Have you thought on anyway to persist the variable references even after re-launch?

Edit: The error they show is:
image

@rxlecky
Copy link
Author

rxlecky commented Nov 12, 2019

indeed the crash does not seem to happen anymore!

That's a great news! So we now know where the error was coming from. The reason why I didn't do those checks was that I was certain all those variables are non-null under normal circumstances. It would be really helpful if you could let me know which of the variables are reported to be null when the ERR_FAIL is triggered.

Also, I was very impressed that I could watch expressions like array.size() and it updated live, it is just amazing!

Haha, really glad to hear that! 😄 Normally IDEs don't support live update of watches, presumably for performance reasons. And the performance was also Juan's concern but he agreed that it is a really useful feature to have so he gave it a greenlight. It would be especially useful if you could test whether you can observe FPS differences between when you are using watches versus when you are not using them. There's also an option in the Editor Settings debugger/watches_refresh_interval which dictates how often the watches UI updates in editor - just in case you'd need more precise UI. Eventually I'd like to make this setting also control how often the watches are evaluated themselves, currently the evalutaion happens on every line of code.

I also noticed something: When canceling the execution of the game, the watched expressions are still there, which is great. However, when playing the game again, they stop working. I guess because the instance ids are gone. Have you thought on anyway to persist the variable references even after re-launch?

Yes, that's exactly the reason why they are not working. There are two reasons why they are not cleared:

  1. The watches can be completely context-independent and there is no reason not to keep those around even after game restarts. Currently this is the case for expressions consisting solely of literals, built-in constants (PI, TAU, ...) and built-in functions. This is not really that useful but will make sense once autoloads are working.

  2. You can still manually "reconnect" the watch with a new context. You need to set up the breakpoint and evaluate the watch there again and then it should continue to function. If the watches are not cleared upon restart, you don't have to type the expression again to reconnect the watch.

That said, manually reconnecting watches is really, REALLY tedious. So the plan is to eventually implement a feature to reconnect watches automatically. This obviously isn't possible for all watches but covering the most common use case would be really helpful already. Most likely the automatic reconnecting would only cover the nodes that are placed in the scene statically, not the ones spawned on runtime.

Edit: The error they show is:

Yes, I am aware that the error messages aren't clear at all. This is one of the UX improvements that I will work on at some point. It isn't really a priority at the moment, though.

@jahd2602
Copy link
Member

@rxlecky yes, with autoloads it would be very useful that it auto-connects automatically.

I have another idea for the automatic reconnection: Keep track of the script and line-number that the watcher started tracking a variable. Then, in a following execution, when the parser reaches that script and line-number, then reconnect the variable. It should probably exist with the same name (if the user has not changed it in between executions).

It would probably cause issues when multiple instances uses the same script (it would only connect with the first instance) but at least it would attend the majority of the cases automatically.

@rxlecky rxlecky force-pushed the javary-script-watches branch 3 times, most recently from 694e346 to 9235ea1 Compare November 15, 2019 01:35
@vnen vnen removed their request for review January 9, 2020 13:52
@rxlecky rxlecky force-pushed the javary-script-watches branch from 9235ea1 to abc2ea0 Compare April 10, 2020 21:16
rxlecky and others added 3 commits April 10, 2020 23:37
Evaluator allows  expressions to be evaluated in the current stack frame.
Watches keep track of a list of expressions.
Game occasionally crashes on nullptr dereference in _evaluate_watches.
Add null checks to help discover the issue.
Evaluating in watches still crashes
@rxlecky rxlecky force-pushed the javary-script-watches branch from abc2ea0 to 093a366 Compare April 10, 2020 21:37
rxlecky added 7 commits April 11, 2020 00:08
Move watches next to the stack frames and the code stepping buttons
Replaced evaluete_watches mutex with atomic counter to prevent infinite
recursion that caused this crash. Also moved all locking from
_evaluate_watches to the inlined evaluate_watches, making _evaluate_watches
thread unsafe.

Fix #12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants